va_arg: pass in TyAndLayout - #163021

…=adwinwhite

`va_arg`: pass in `TyAndLayout`

Just a refactor, no functional changes. It is weird I did not spot this before, it cleans up the code a bunch.

…uwer

Rollup of 15 pull requests

Successful merges:

 - #162726 (std: fix unix socket address panic on a full sun_path)
 - #163016 (Don't claim that escaping value is a reference in diagnostics)
 - #163040 (Tweak "use array's length as const param" suggestion)
 - #163060 (Point to fields that introduce trait requirements)
 - #163066 (don't mark `f128` as reliable on AIX)
 - #162098 (Tweak `Infallible` docs)
 - #162854 (Add safety section for atomic_load/store)
 - #163015 (add `minicore::ffi::VaList`)
 - #163021 (`va_arg`: pass in `TyAndLayout`)
 - #163036 ([rustdoc] Correctly handle `dyn` trait methods linking for jump to def feature)
 - #163042 (Remove redundant output from suggestion)
 - #163046 (Use verbose suggestion for `const _`)
 - #163050 (Use verbose suggestion for similarly named label suggestion)
 - #163052 (Use verbose suggestion for wrong primitive type names)
 - #163055 (Use the full path of `bug_impl` to avoid bogus errors in rust-analyzer)

…uwer

Rollup of 15 pull requests

Successful merges:

 - #162726 (std: fix unix socket address panic on a full sun_path)
 - #163016 (Don't claim that escaping value is a reference in diagnostics)
 - #163040 (Tweak "use array's length as const param" suggestion)
 - #163060 (Point to fields that introduce trait requirements)
 - #163066 (don't mark `f128` as reliable on AIX)
 - #162098 (Tweak `Infallible` docs)
 - #162854 (Add safety section for atomic_load/store)
 - #163015 (add `minicore::ffi::VaList`)
 - #163021 (`va_arg`: pass in `TyAndLayout`)
 - #163036 ([rustdoc] Correctly handle `dyn` trait methods linking for jump to def feature)
 - #163042 (Remove redundant output from suggestion)
 - #163046 (Use verbose suggestion for `const _`)
 - #163050 (Use verbose suggestion for similarly named label suggestion)
 - #163052 (Use verbose suggestion for wrong primitive type names)
 - #163055 (Use the full path of `bug_impl` to avoid bogus errors in rust-analyzer)

…uwer

Rollup of 15 pull requests

Successful merges:

 - #162726 (std: fix unix socket address panic on a full sun_path)
 - #163016 (Don't claim that escaping value is a reference in diagnostics)
 - #163040 (Tweak "use array's length as const param" suggestion)
 - #163060 (Point to fields that introduce trait requirements)
 - #163066 (don't mark `f128` as reliable on AIX)
 - #162098 (Tweak `Infallible` docs)
 - #162854 (Add safety section for atomic_load/store)
 - #163015 (add `minicore::ffi::VaList`)
 - #163021 (`va_arg`: pass in `TyAndLayout`)
 - #163036 ([rustdoc] Correctly handle `dyn` trait methods linking for jump to def feature)
 - #163042 (Remove redundant output from suggestion)
 - #163046 (Use verbose suggestion for `const _`)
 - #163050 (Use verbose suggestion for similarly named label suggestion)
 - #163052 (Use verbose suggestion for wrong primitive type names)
 - #163055 (Use the full path of `bug_impl` to avoid bogus errors in rust-analyzer)

…uwer

Rollup of 15 pull requests

Successful merges:

 - #162726 (std: fix unix socket address panic on a full sun_path)
 - #163016 (Don't claim that escaping value is a reference in diagnostics)
 - #163040 (Tweak "use array's length as const param" suggestion)
 - #163060 (Point to fields that introduce trait requirements)
 - #163066 (don't mark `f128` as reliable on AIX)
 - #162098 (Tweak `Infallible` docs)
 - #162854 (Add safety section for atomic_load/store)
 - #163015 (add `minicore::ffi::VaList`)
 - #163021 (`va_arg`: pass in `TyAndLayout`)
 - #163036 ([rustdoc] Correctly handle `dyn` trait methods linking for jump to def feature)
 - #163042 (Remove redundant output from suggestion)
 - #163046 (Use verbose suggestion for `const _`)
 - #163050 (Use verbose suggestion for similarly named label suggestion)
 - #163052 (Use verbose suggestion for wrong primitive type names)
 - #163055 (Use the full path of `bug_impl` to avoid bogus errors in rust-analyzer)

Rollup merge of #163021 - folkertdev:va-arg-ty-and-layout, r=adwinwhite

`va_arg`: pass in `TyAndLayout`

Just a refactor, no functional changes. It is weird I did not spot this before, it cleans up the code a bunch.

fix and test `va_arg` on `f128` on `x86`

tracking issue: rust-lang#116909

Or well, maybe not fix but clarify. There are no actual behavioral changes, but I do think the new code is more correct.

The clang logic for `va_arg` is kind of (weirdly) complicated. `EmitVAArg` looks simple, but the complexity is hiding in the "messing with TypeInfo" bit:

https://github.com/llvm/llvm-project/blob/0e2786dcb5b28f754a1144c27e708d1c42676af9/clang/lib/CodeGen/Targets/X86.cpp#L1080-L1101

```c++
RValue X86_32ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                                QualType Ty, AggValueSlot Slot) const {

  auto TypeInfo = getContext().getTypeInfoInChars(Ty);

  CCState State(*const_cast<CGFunctionInfo *>(CGF.CurFnInfo));
  ABIArgInfo AI = classifyArgumentType(Ty, State, /*ArgIndex*/ 0);
  // Empty records are ignored for parameter passing purposes.
  if (AI.isIgnore())
    return Slot.asRValue();

  // x86-32 changes the alignment of certain arguments on the stack.
  //
  // Just messing with TypeInfo like this works because we never pass
  // anything indirectly.
  TypeInfo.Align = CharUnits::fromQuantity(
                getTypeStackAlignInBytes(Ty, TypeInfo.Align.getQuantity()));

  return emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false, TypeInfo,
                          CharUnits::fromQuantity(4),
                          /*AllowHigherAlign*/ true, Slot);
}
```

Relevant for us is that it makes an exception for `f128`, for which a higher align (16, instead of the slot size of 4) is used.

https://github.com/llvm/llvm-project/blob/0e2786dcb5b28f754a1144c27e708d1c42676af9/clang/lib/CodeGen/Targets/X86.cpp#L575-L583

```c++
unsigned X86_32ABIInfo::getTypeStackAlignInBytes(QualType Ty,
                                                 unsigned Align) const {
  // Otherwise, if the alignment is less than or equal to the minimum ABI
  // alignment, just use the default; the backend will handle this.
  if (Align <= MinABIStackAlignInBytes)
    return 0; // Use default alignment.

  if (Ty->isFloat128Type())
    return 16;
```

Our code does not have this `TypeInfo` idea, and it's not correct in general to just use `AllowHigherAlign::Yes`. But using it only for `f128` works. We don't need to consider more complicated cases like structs containing `f128` fields.

The code here used `is_like_windows` before. I'm not exactly sure why, the behavior is the same for all accepted types, except now `f128`. But windows does not have that type, so its (c-variadic) ABI is just an LLVM fabrication. The x86 code does not seem to make a distinction based on the ABI, so having the same behavior across targets seems best to me.

`va_arg` to fetch an `i128` argument is currently broken in clang. `i128` on the target is apparently a clang extension anyway, so kind of low priority. `f128` I only fixed somewhat recently in llvm/llvm-project#218017.

r? beetrees or @tgross35

(this may cause a small conflict with rust-lang#163021)

fix and test `va_arg` on `f128` on `x86`

tracking issue: rust-lang#116909

Or well, maybe not fix but clarify. There are no actual behavioral changes, but I do think the new code is more correct.

The clang logic for `va_arg` is kind of (weirdly) complicated. `EmitVAArg` looks simple, but the complexity is hiding in the "messing with TypeInfo" bit:

https://github.com/llvm/llvm-project/blob/0e2786dcb5b28f754a1144c27e708d1c42676af9/clang/lib/CodeGen/Targets/X86.cpp#L1080-L1101

```c++
RValue X86_32ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                                QualType Ty, AggValueSlot Slot) const {

  auto TypeInfo = getContext().getTypeInfoInChars(Ty);

  CCState State(*const_cast<CGFunctionInfo *>(CGF.CurFnInfo));
  ABIArgInfo AI = classifyArgumentType(Ty, State, /*ArgIndex*/ 0);
  // Empty records are ignored for parameter passing purposes.
  if (AI.isIgnore())
    return Slot.asRValue();

  // x86-32 changes the alignment of certain arguments on the stack.
  //
  // Just messing with TypeInfo like this works because we never pass
  // anything indirectly.
  TypeInfo.Align = CharUnits::fromQuantity(
                getTypeStackAlignInBytes(Ty, TypeInfo.Align.getQuantity()));

  return emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false, TypeInfo,
                          CharUnits::fromQuantity(4),
                          /*AllowHigherAlign*/ true, Slot);
}
```

Relevant for us is that it makes an exception for `f128`, for which a higher align (16, instead of the slot size of 4) is used.

https://github.com/llvm/llvm-project/blob/0e2786dcb5b28f754a1144c27e708d1c42676af9/clang/lib/CodeGen/Targets/X86.cpp#L575-L583

```c++
unsigned X86_32ABIInfo::getTypeStackAlignInBytes(QualType Ty,
                                                 unsigned Align) const {
  // Otherwise, if the alignment is less than or equal to the minimum ABI
  // alignment, just use the default; the backend will handle this.
  if (Align <= MinABIStackAlignInBytes)
    return 0; // Use default alignment.

  if (Ty->isFloat128Type())
    return 16;
```

Our code does not have this `TypeInfo` idea, and it's not correct in general to just use `AllowHigherAlign::Yes`. But using it only for `f128` works. We don't need to consider more complicated cases like structs containing `f128` fields.

The code here used `is_like_windows` before. I'm not exactly sure why, the behavior is the same for all accepted types, except now `f128`. But windows does not have that type, so its (c-variadic) ABI is just an LLVM fabrication. The x86 code does not seem to make a distinction based on the ABI, so having the same behavior across targets seems best to me.

`va_arg` to fetch an `i128` argument is currently broken in clang. `i128` on the target is apparently a clang extension anyway, so kind of low priority. `f128` I only fixed somewhat recently in llvm/llvm-project#218017.

r? beetrees or @tgross35

(this may cause a small conflict with rust-lang#163021)

Rollup merge of #163037 - folkertdev:x86-vaarg-f128, r=beetrees

fix and test `va_arg` on `f128` on `x86`

tracking issue: #116909

Or well, maybe not fix but clarify. There are no actual behavioral changes, but I do think the new code is more correct.

The clang logic for `va_arg` is kind of (weirdly) complicated. `EmitVAArg` looks simple, but the complexity is hiding in the "messing with TypeInfo" bit:

https://github.com/llvm/llvm-project/blob/0e2786dcb5b28f754a1144c27e708d1c42676af9/clang/lib/CodeGen/Targets/X86.cpp#L1080-L1101

```c++
RValue X86_32ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
                                QualType Ty, AggValueSlot Slot) const {

  auto TypeInfo = getContext().getTypeInfoInChars(Ty);

  CCState State(*const_cast<CGFunctionInfo *>(CGF.CurFnInfo));
  ABIArgInfo AI = classifyArgumentType(Ty, State, /*ArgIndex*/ 0);
  // Empty records are ignored for parameter passing purposes.
  if (AI.isIgnore())
    return Slot.asRValue();

  // x86-32 changes the alignment of certain arguments on the stack.
  //
  // Just messing with TypeInfo like this works because we never pass
  // anything indirectly.
  TypeInfo.Align = CharUnits::fromQuantity(
                getTypeStackAlignInBytes(Ty, TypeInfo.Align.getQuantity()));

  return emitVoidPtrVAArg(CGF, VAListAddr, Ty, /*Indirect*/ false, TypeInfo,
                          CharUnits::fromQuantity(4),
                          /*AllowHigherAlign*/ true, Slot);
}
```

Relevant for us is that it makes an exception for `f128`, for which a higher align (16, instead of the slot size of 4) is used.

https://github.com/llvm/llvm-project/blob/0e2786dcb5b28f754a1144c27e708d1c42676af9/clang/lib/CodeGen/Targets/X86.cpp#L575-L583

```c++
unsigned X86_32ABIInfo::getTypeStackAlignInBytes(QualType Ty,
                                                 unsigned Align) const {
  // Otherwise, if the alignment is less than or equal to the minimum ABI
  // alignment, just use the default; the backend will handle this.
  if (Align <= MinABIStackAlignInBytes)
    return 0; // Use default alignment.

  if (Ty->isFloat128Type())
    return 16;
```

Our code does not have this `TypeInfo` idea, and it's not correct in general to just use `AllowHigherAlign::Yes`. But using it only for `f128` works. We don't need to consider more complicated cases like structs containing `f128` fields.

The code here used `is_like_windows` before. I'm not exactly sure why, the behavior is the same for all accepted types, except now `f128`. But windows does not have that type, so its (c-variadic) ABI is just an LLVM fabrication. The x86 code does not seem to make a distinction based on the ABI, so having the same behavior across targets seems best to me.

`va_arg` to fetch an `i128` argument is currently broken in clang. `i128` on the target is apparently a clang extension anyway, so kind of low priority. `f128` I only fixed somewhat recently in llvm/llvm-project#218017.

r? beetrees or @tgross35

(this may cause a small conflict with #163021)